refactor: replace SubProject inheritance with has-a Fetcher composition#1170
Open
refactor: replace SubProject inheritance with has-a Fetcher composition#1170
Conversation
SubProject was an ABC mixing VCS-specific capabilities with domain orchestration. Archive fetching has no concept of branches/tags/revisions, so forcing it to implement VCS-shaped stubs was a design smell. New domain model: - Fetcher (Protocol): minimal common contract (fetch, freeze, wanted_version, latest_available_version, list_tool_info) - VcsFetcher(Fetcher, Protocol): VCS additions (branches, tags, revisions, browse_tree, patch_type) — git and svn only - AbstractVcsFetcher(ABC): shared latest_available_version + freeze logic, eliminating duplication between GitFetcher and SvnFetcher - ArchiveFetcher: implements Fetcher only — no VCS methods - SubProject: concrete domain aggregate composed with a Fetcher; single dispatch point via as_vcs() -> VcsFetcher | None All cyclomatic complexity kept ≤ 6. Pylint 10.00/10. All 569 unit tests pass. Feature tests pass for git; SVN failures are environmental (no svn). https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughRefactors SubProject into a fetcher-based design: adds Fetcher/VcsFetcher/AbstractVcsFetcher, converts Git/SVN/Archive subprojects to fetchers, makes SubProject an orchestrator accepting a Fetcher, and updates command, terminal, logging, and tests to use Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Command
participant Sub as SubProject
participant Fetch as VcsFetcher
participant Remote as RemoteRepo
CLI->>Sub: request browse / patch-type / fetch
Sub->>Fetch: as_vcs()\nbrowse_tree(version) / patch_type() / fetch(version, local_path,...)
Fetch->>Remote: VCS operations (list branches/tags, tree, export/clone, download)
Remote-->>Fetch: entries / revision / archive data
Fetch-->>Sub: resolved Version / entries / dependencies
Sub-->>CLI: deliver result (entries, patch type, updated metadata)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 48 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_subproject.py (1)
175-196: 🧹 Nitpick | 🔵 TrivialConsider reducing context manager nesting with
contextlib.ExitStack.Six levels of nested
withstatements reduce readability. UsingExitStackorpytest-mock'smockerfixture would flatten this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_subproject.py` around lines 175 - 196, Replace the six nested with-patch context managers in the test with a single contextlib.ExitStack to improve readability: import ExitStack and use "with ExitStack() as stack:" then enter each patch via stack.enter_context(patch("dfetch.project.subproject.os.path.exists")) (and likewise for "dfetch.project.subproject.Metadata.from_file", "dfetch.project.subproject.hash_directory", "dfetch.project.subproject.safe_rm", "dfetch.project.metadata.Metadata.fetched", "dfetch.project.metadata.Metadata.dump"), keep the same mock return values and the rest of the test exercising _make_subproject(...).update(...) and assertions unchanged so mock_hash.call_args still inspects the "skiplist".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/project/__init__.py`:
- Line 41: The RuntimeError raised ("vcs type unsupported") should include the
remote URL to aid debugging; update the raise in dfetch.project.__init__.py to
interpolate the remote URL variable in scope (e.g., url, remote_url, or remote)
into the error message and provide brief context like "vcs type unsupported for
<remote_url>" so developers can see which manifest entry failed when the
exception is raised.
In `@dfetch/project/gitsubproject.py`:
- Around line 41-44: The inline "# type: ignore" on the `@lru_cache` decorator of
get_default_branch hides a typing/design problem: lru_cache on an instance
method shares cache across instances. Replace the decorator with
functools.cached_property (or functools.cached_property if backport present) on
the get_default_branch method so the default branch is computed and cached
per-instance, or if you intentionally want a cross-instance cache, remove the
type-ignore and add a clear docstring on get_default_branch describing the
shared-cache behavior and change `@lru_cache` to `@functools.lru_cache`(maxsize=...)
to make intent explicit; update imports to use functools.cached_property or
functools.lru_cache accordingly and remove the "# type: ignore" comment.
In `@pyproject.toml`:
- Around line 158-160: The two functions currently using inline "# pylint:
disable" waivers (the add-command handler in add.py around the complaint at line
~304 and the pick helper in pick.py around line ~77) exceed the configured
max-args; remove the inline suppressions and refactor those functions to accept
≤6 parameters by grouping related parameters into small dataclasses/objects or a
single context/config object, breaking large functions into smaller helpers that
take fewer arguments, or moving optional args into keyword-only config objects;
after refactoring, delete the inline "# pylint: disable" comments so the
functions conform to the pyproject.toml design thresholds.
In `@tests/test_subproject.py`:
- Around line 68-72: The factory function _make_subproject has an imprecise type
for the wanted parameter; update its signature to explicitly allow None by
changing wanted: Version = None to wanted: Version | None = None (or
Optional[Version] if using typing imports) so the function return typing is
accurate; adjust any related annotations/tests that rely on this signature and
ensure Version is imported/available in the scope where _make_subproject is
defined.
- Around line 24-25: Update the __init__ signature to explicitly annotate the
optional parameter as Version | None instead of just using a default of None;
locate the __init__ method on the class in tests/test_subproject.py (the method
that sets self._wanted) and change the parameter annotation from wanted: Version
= None to wanted: Version | None so static type checkers recognize None is
allowed, leaving the default and internal assignment (self._wanted = wanted or
Version()) unchanged.
---
Outside diff comments:
In `@tests/test_subproject.py`:
- Around line 175-196: Replace the six nested with-patch context managers in the
test with a single contextlib.ExitStack to improve readability: import ExitStack
and use "with ExitStack() as stack:" then enter each patch via
stack.enter_context(patch("dfetch.project.subproject.os.path.exists")) (and
likewise for "dfetch.project.subproject.Metadata.from_file",
"dfetch.project.subproject.hash_directory", "dfetch.project.subproject.safe_rm",
"dfetch.project.metadata.Metadata.fetched",
"dfetch.project.metadata.Metadata.dump"), keep the same mock return values and
the rest of the test exercising _make_subproject(...).update(...) and assertions
unchanged so mock_hash.call_args still inspects the "skiplist".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f45e5c4-05b0-46e3-9d5d-ca89a93beda2
📒 Files selected for processing (16)
dfetch/commands/add.pydfetch/commands/environment.pydfetch/commands/format_patch.pydfetch/project/__init__.pydfetch/project/archivesubproject.pydfetch/project/fetcher.pydfetch/project/gitsubproject.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/project/svnsubproject.pydfetch/project/svnsuperproject.pypyproject.tomltests/test_add.pytests/test_archive.pytests/test_subproject.pytests/test_svn.py
- __init__.py: include remote URL in 'vcs type unsupported' RuntimeError so developers can identify which manifest entry triggered the failure - gitsubproject.py: replace @lru_cache + type: ignore with explicit per-instance lazy cache (_default_branch field), eliminating the shared-across-instances cache issue and the type suppression - pick.py: remove redundant 'n' parameter from _render_pick_lines; compute len(items) inside the function, dropping the too-many-arguments suppression comment - add.py: extract remote.set_remote() call out of _build_entry so the function stays within the 6-argument limit; apply it at each call site, dropping the too-many-arguments suppression comment - test_subproject.py: tighten Version | None annotations on MockVcsFetcher __init__ and _make_subproject; replace six nested with-patch managers with a single contextlib.ExitStack for readability https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
- Add raise NotImplementedError to Protocol methods with non-None return types so pyright accepts the bodies without triggering pylint W2301 - Annotate NAME: str on all concrete fetchers and MockVcsFetcher to satisfy pyright's Protocol invariance check - Rename SvnFetcher.fetch _name -> name (Protocol parameter match); emit a debug log to silence the unused-argument warning - Extract _resolve_entry_version helper from _non_interactive_entry in add.py to reduce cyclomatic complexity from C(11) to B(8) - Fix mypy errors: explicit tuple unpack in license.py, str() casts on Prompt.ask() calls in prompt.py and add.py - Use TYPE_CHECKING stub for LogRender base to avoid "subclass of Any" mypy error without requiring an inline suppression comment https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/commands/add.py`:
- Around line 275-285: The resolver _resolve_entry_version should branch on
ctx.subproject.as_vcs() and avoid using VCS branch/tag lookup for archive-backed
subprojects; update _resolve_entry_version (and the similar logic around lines
290-294) so that if ctx.subproject.as_vcs() is True you use the existing
branches/tags + _resolve_raw_version behavior, but if False you either parse the
raw_version with archive-specific logic provided by the subproject (e.g., an
archive-version parser on ctx.subproject) or explicitly reject/raise for
unsupported --version inputs, returning a Version that preserves the archive
identifier (not a branch: field) when accepting archive versions. Ensure you
reference Version and _resolve_raw_version only for VCS subprojects and do not
fall back to Version(branch=raw_version) for archives.
In `@dfetch/log.py`:
- Around line 21-30: Remove the inline "# pylint:
disable=too-few-public-methods" comments by replacing the small adapter classes
with a function-based wrapper around the existing LogRender type: remove class
_LogRenderBase and its inline suppression and instead provide a factory function
(e.g., make_log_renderer or wrap_log_render) that returns a callable
implementing the same behavior as _LogRenderBase.__init__ and
_LogRenderBase.__call__, and replace _NoExpandLogRender class usage with a
function that returns the adapted LogRender callable; ensure references to
_LogRenderBase, _NoExpandLogRender, __init__, and __call__ are updated to use
the new wrapper so no inline pylint suppression is required.
In `@dfetch/project/archivesubproject.py`:
- Around line 101-125: The fetch() path currently uses the original URL as the
returned Version for non-hash archives, allowing a later freeze() to re-download
a possibly-changed remote; change fetch() to capture the actual downloaded
content digest from the download/verify step and return a Version pinned to that
digest so the on-disk tree matches the recorded pin. Concretely: make
_download_and_verify (or the helper it calls) return the computed digest (or
expose it on the Archivesubproject instance), then in fetch() call that,
build/replace the returned Version.revision with the computed digest (instead of
leaving the URL), and return that updated Version; apply the same change to the
analogous code path referenced for lines 141-161 so both fetch code paths record
and return the fetched content digest rather than the original remote URL.
- Around line 97-99: latest_available_version currently returns wanted if the
archive URL is reachable, which wrongly reports hash-pinned archives as
available; change latest_available_version so that when an integrity.hash is
present for the project (the integrity field / integrity.hash), it does not rely
on is_accessible() alone but verifies the remote bytes match the pinned digest
before returning wanted — e.g. call a verification routine (download/stream and
compute the same digest, or use a remote-repo verify method if available) and
only return wanted when the computed digest equals integrity.hash, otherwise
return None (and treat transient errors as not-available); this prevents callers
(and fetch()) from being misled by reachability alone.
In `@dfetch/project/fetcher.py`:
- Around line 126-134: latest_available_version currently collapses
revision-only pins into a branch lookup (trunk) which breaks SVN pinned
revisions; update the logic so revision-only requests are preserved: either
modify latest_available_version (the method shown) to return
self._revision_version_if_exists(wanted.revision) when
self._is_revision_only(wanted) without depending on revision_is_enough(), or
implement an override in SvnFetcher (create SvnFetcher.latest_available_version)
that checks _is_revision_only and returns the specific revision via
_revision_version_if_exists; reference _is_revision_only,
_revision_version_if_exists, latest_available_version and SvnFetcher.fetch to
ensure SvnFetcher continues to accept Version(revision=...) unchanged.
In `@tests/test_subproject.py`:
- Around line 50-51: MockVcsFetcher.browse_tree currently returns None, so tests
that call with sp.as_vcs().browse_tree(...) fail; change
MockVcsFetcher.browse_tree to return a real context manager (e.g., use
contextlib.contextmanager or contextlib.nullcontext) that yields an appropriate
dummy tree/temporary path object expected by consumers; ensure the returned
context manager implements __enter__/__exit__ and yields the same type/shape
that SubProject.as_vcs() consumers expect so tests using with
sp.as_vcs().browse_tree(...) work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a968269-6a6f-4270-bdbc-37c18720535d
📒 Files selected for processing (11)
dfetch/commands/add.pydfetch/log.pydfetch/project/archivesubproject.pydfetch/project/fetcher.pydfetch/project/gitsubproject.pydfetch/project/subproject.pydfetch/project/svnsubproject.pydfetch/terminal/pick.pydfetch/terminal/prompt.pydfetch/util/license.pytests/test_subproject.py
Comment on lines
+275
to
+285
| def _resolve_entry_version(ctx: _AddContext, raw_version: str) -> Version: | ||
| """Resolve a raw version string to a ``Version`` using remote branches and tags.""" | ||
| branches = ctx.subproject.list_of_branches() | ||
| tags = ctx.subproject.list_of_tags() | ||
| choices: list[Version] = [ | ||
| *[Version(branch=b) for b in prioritise_default(branches, ctx.default_branch)], | ||
| *[Version(tag=t) for t in sort_tags_newest_first(tags)], | ||
| ] | ||
| return _resolve_raw_version(raw_version, choices) or Version( | ||
| branch=ctx.default_branch | ||
| ) |
Contributor
There was a problem hiding this comment.
Handle archive --version values separately.
This resolver now runs for every fetcher, but archive-backed subprojects expose no branches or tags. In that case any non-Git-SHA input falls through to Version(branch=raw_version), so dfetch add ... --version ... can emit a VCS-style branch: field for an archive dependency instead of preserving the archive identifier/hash. Please branch this on ctx.subproject.as_vcs() and either parse archive versions through archive-specific logic or reject --version for archive URLs.
Also applies to: 290-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/commands/add.py` around lines 275 - 285, The resolver
_resolve_entry_version should branch on ctx.subproject.as_vcs() and avoid using
VCS branch/tag lookup for archive-backed subprojects; update
_resolve_entry_version (and the similar logic around lines 290-294) so that if
ctx.subproject.as_vcs() is True you use the existing branches/tags +
_resolve_raw_version behavior, but if False you either parse the raw_version
with archive-specific logic provided by the subproject (e.g., an archive-version
parser on ctx.subproject) or explicitly reject/raise for unsupported --version
inputs, returning a Version that preserves the archive identifier (not a branch:
field) when accepting archive versions. Ensure you reference Version and
_resolve_raw_version only for VCS subprojects and do not fall back to
Version(branch=raw_version) for archives.
Comment on lines
+21
to
+30
| class _LogRenderBase: # pylint: disable=too-few-public-methods | ||
| def __init__(self, **_kwargs: Any) -> None: ... | ||
|
|
||
| def __call__(self, *_args: Any, **_kwargs: Any) -> Any: ... | ||
|
|
||
| else: | ||
| _LogRenderBase = LogRender | ||
|
|
||
|
|
||
| class _NoExpandLogRender(_LogRenderBase): # pylint: disable=too-few-public-methods |
Contributor
There was a problem hiding this comment.
Remove inline pylint suppressions in new logging adapter types
The new # pylint: disable=too-few-public-methods comments violate the repository rule against inline suppressions. Refactor this to a function-based wrapper so no suppression is needed.
Proposed refactor (no inline suppressions)
-if TYPE_CHECKING:
-
- class _LogRenderBase: # pylint: disable=too-few-public-methods
- def __init__(self, **_kwargs: Any) -> None: ...
-
- def __call__(self, *_args: Any, **_kwargs: Any) -> Any: ...
-
-else:
- _LogRenderBase = LogRender
-
-
-class _NoExpandLogRender(_LogRenderBase): # pylint: disable=too-few-public-methods
- """LogRender that disables table expansion to prevent blank lines in asciicasts."""
-
- def __call__(self, *args: Any, **kwargs: Any) -> Any:
- """Render log entry without expanding the table to the full terminal width."""
- table = super().__call__(*args, **kwargs)
- table.expand = False
- return table
+def _make_no_expand_log_render(**kwargs: Any) -> Any:
+ """Create a LogRender callable that disables table expansion."""
+ delegate = LogRender(**kwargs)
+
+ def _render(*args: Any, **inner_kwargs: Any) -> Any:
+ table = delegate(*args, **inner_kwargs)
+ table.expand = False
+ return table
+
+ return _render- handler._log_render = _NoExpandLogRender( # pylint: disable=protected-access
+ handler._log_render = _make_no_expand_log_render( # pylint: disable=protected-access
show_time=False, show_level=False, show_path=False
)As per coding guidelines: "Do not use inline lint suppressions (# noqa, # type: ignore, # pylint: disable, # pyright: ignore) without fixing the root cause."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/log.py` around lines 21 - 30, Remove the inline "# pylint:
disable=too-few-public-methods" comments by replacing the small adapter classes
with a function-based wrapper around the existing LogRender type: remove class
_LogRenderBase and its inline suppression and instead provide a factory function
(e.g., make_log_renderer or wrap_log_render) that returns a callable
implementing the same behavior as _LogRenderBase.__init__ and
_LogRenderBase.__call__, and replace _NoExpandLogRender class usage with a
function that returns the adapted LogRender callable; ensure references to
_LogRenderBase, _NoExpandLogRender, __init__, and __call__ are updated to use
the new wrapper so no inline pylint suppression is required.
Comment on lines
+97
to
+99
| def latest_available_version(self, wanted: Version) -> Version | None: | ||
| """Return *wanted* if the archive URL is still reachable, else None.""" | ||
| return wanted if self._remote_repo.is_accessible() else None |
Contributor
There was a problem hiding this comment.
Don't report hash-pinned archives as available on reachability alone.
For integrity.hash entries this returns wanted whenever the URL responds, even if the bytes at that URL no longer match the pinned digest. fetch() will then fail with a hash mismatch, so callers can treat an unfetchable version as available.
Suggested fix
def latest_available_version(self, wanted: Version) -> Version | None:
"""Return *wanted* if the archive URL is still reachable, else None."""
- return wanted if self._remote_repo.is_accessible() else None
+ expected = IntegrityHash.parse(wanted.revision)
+ if expected is None:
+ return wanted if self._remote_repo.is_accessible() else None
+
+ with temp_file(_suffix_for_url(self._remote)) as tmp_path:
+ actual_hex = self._remote_repo.download(
+ tmp_path, algorithm=expected.algorithm
+ )
+ return wanted if expected.matches(actual_hex) else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/archivesubproject.py` around lines 97 - 99,
latest_available_version currently returns wanted if the archive URL is
reachable, which wrongly reports hash-pinned archives as available; change
latest_available_version so that when an integrity.hash is present for the
project (the integrity field / integrity.hash), it does not rely on
is_accessible() alone but verifies the remote bytes match the pinned digest
before returning wanted — e.g. call a verification routine (download/stream and
compute the same digest, or use a remote-repo verify method if available) and
only return wanted when the computed digest equals integrity.hash, otherwise
return None (and treat transient errors as not-available); this prevents callers
(and fetch()) from being misled by reachability alone.
Comment on lines
+101
to
125
| def fetch( | ||
| self, | ||
| version: Version, | ||
| local_path: str, | ||
| name: str, | ||
| source: str, | ||
| ignore: Sequence[str], | ||
| ) -> tuple[Version, list[Dependency]]: | ||
| """Download and extract the archive to *local_path*. | ||
|
|
||
| Raises: | ||
| RuntimeError: On download failure or hash mismatch. | ||
|
|
||
| Returns: | ||
| The version that was actually fetched (hash string or URL). | ||
| """ | ||
| revision = version.revision | ||
|
|
||
| pathlib.Path(self.local_path).mkdir(parents=True, exist_ok=True) | ||
|
|
||
| with temp_file(_suffix_for_url(self.remote)) as tmp_path: | ||
| expected = IntegrityHash.parse(revision) | ||
| if expected: | ||
| actual_hex = self._remote_repo.download( | ||
| tmp_path, algorithm=expected.algorithm | ||
| ) | ||
| if not expected.matches(actual_hex): | ||
| raise RuntimeError( | ||
| f"Hash mismatch for {self._project_entry.name}! " | ||
| f"{expected.algorithm} expected {expected.hex_digest}" | ||
| ) | ||
| else: | ||
| self._remote_repo.download(tmp_path) | ||
| pathlib.Path(local_path).mkdir(parents=True, exist_ok=True) | ||
|
|
||
| with temp_file(_suffix_for_url(self._remote)) as tmp_path: | ||
| self._download_and_verify(version.revision, tmp_path, name) | ||
| ArchiveLocalRepo.extract( | ||
| tmp_path, | ||
| self.local_path, | ||
| src=self.source, | ||
| ignore=self.ignore, | ||
| local_path, | ||
| src=source, | ||
| ignore=ignore, | ||
| ) | ||
|
|
||
| return version, [] |
Contributor
There was a problem hiding this comment.
Freeze can pin different bytes than the tree you just fetched.
For non-hash archives, fetch() returns the original URL as the on-disk version, and freeze() later re-downloads that URL to compute the pin. If the remote changes between those two steps, the manifest gets pinned to a different archive than the one extracted locally. Record the fetched content digest during fetch() and reuse that here instead of re-fetching.
Also applies to: 141-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/archivesubproject.py` around lines 101 - 125, The fetch() path
currently uses the original URL as the returned Version for non-hash archives,
allowing a later freeze() to re-download a possibly-changed remote; change
fetch() to capture the actual downloaded content digest from the download/verify
step and return a Version pinned to that digest so the on-disk tree matches the
recorded pin. Concretely: make _download_and_verify (or the helper it calls)
return the computed digest (or expose it on the Archivesubproject instance),
then in fetch() call that, build/replace the returned Version.revision with the
computed digest (instead of leaving the URL), and return that updated Version;
apply the same change to the analogous code path referenced for lines 141-161 so
both fetch code paths record and return the fetched content digest rather than
the original remote URL.
Comment on lines
+126
to
+134
| def latest_available_version(self, wanted: Version) -> Version | None: | ||
| """Return the latest version matching *wanted*, or None if unavailable.""" | ||
| if wanted.tag: | ||
| return self._latest_tag_version(wanted.tag) | ||
| if self._is_revision_only(wanted): | ||
| return self._revision_version_if_exists(wanted.revision) | ||
| branch = self._resolve_branch(wanted.branch) | ||
| revision = self.latest_revision_on_branch(branch) | ||
| return Version(revision=revision, branch=branch) if revision else None |
Contributor
There was a problem hiding this comment.
latest_available_version() breaks revision-only SVN pins.
This fallback only preserves a revision-only request when revision_is_enough() is true. SvnFetcher.fetch() still accepts Version(revision=...) by defaulting it to trunk, so a pinned SVN revision is resolved here to the current trunk head instead of the requested revision. dfetch check will therefore report that project as out of date forever. Either reject revision-only SVN inputs before they reach this path, or let SvnFetcher override this method to keep the pinned revision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/fetcher.py` around lines 126 - 134, latest_available_version
currently collapses revision-only pins into a branch lookup (trunk) which breaks
SVN pinned revisions; update the logic so revision-only requests are preserved:
either modify latest_available_version (the method shown) to return
self._revision_version_if_exists(wanted.revision) when
self._is_revision_only(wanted) without depending on revision_is_enough(), or
implement an override in SvnFetcher (create SvnFetcher.latest_available_version)
that checks _is_revision_only and returns the specific revision via
_revision_version_if_exists; reference _is_revision_only,
_revision_version_if_exists, latest_available_version and SvnFetcher.fetch to
ensure SvnFetcher continues to accept Version(revision=...) unchanged.
Comment on lines
+50
to
+51
| def browse_tree(self, version: str) -> object: | ||
| return None |
Contributor
There was a problem hiding this comment.
Return a real context manager from MockVcsFetcher.browse_tree().
SubProject.as_vcs() treats this double as a VcsFetcher, but browse_tree() returns None. Any test that exercises with sp.as_vcs().browse_tree(...) will fail because of the mock instead of the production code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_subproject.py` around lines 50 - 51, MockVcsFetcher.browse_tree
currently returns None, so tests that call with sp.as_vcs().browse_tree(...)
fail; change MockVcsFetcher.browse_tree to return a real context manager (e.g.,
use contextlib.contextmanager or contextlib.nullcontext) that yields an
appropriate dummy tree/temporary path object expected by consumers; ensure the
returned context manager implements __enter__/__exit__ and yields the same
type/shape that SubProject.as_vcs() consumers expect so tests using with
sp.as_vcs().browse_tree(...) work.
add.py: _resolve_entry_version now short-circuits for archive-backed subprojects (as_vcs() is None), returning Version(revision=raw_version) instead of falling through to a VCS branch/tag lookup that always produces an empty branch fallback for archives. log.py: replace _LogRenderBase/_NoExpandLogRender class pair (each requiring a pylint too-few-public-methods suppression) with a single factory function _make_non_expanding_log_render; the only remaining suppression is the pre-existing protected-access on handler._log_render. svnsubproject.py: override latest_available_version so SVN revision-only pins (revision set, no branch, no tag) return the pinned revision with the default branch context; previously the base implementation called latest_revision_on_branch(trunk), causing dfetch check to report pinned SVN revisions as out of date. tests/test_subproject.py: MockVcsFetcher.browse_tree now returns nullcontext(lambda path="": []) so any consumer calling with subproject.as_vcs().browse_tree(...) as ls: gets a valid context manager instead of None. https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
…toring Replace the old SubProject inheritance model (ArchiveSubProject/GitSubproject/ SvnSubproject each implementing abstract SubProject) with the new composition design: SubProject composes with a Fetcher; Fetcher and VcsFetcher as protocols; AbstractVcsFetcher as shared ABC; GitFetcher and SvnFetcher implementing VcsFetcher; ArchiveFetcher implementing Fetcher only. Update AGENTS.md to describe the concrete SubProject aggregate and direct developers to implement Fetcher/VcsFetcher protocols when adding new backends. https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/commands/add.py (1)
87-108:⚠️ Potential issue | 🟠 MajorFall back to text prompts when the subproject has no VCS browser.
For archive-backed subprojects this branch yields an
LsFunctionthat always returns[], but_ask_src()and_ask_ignore()still take the TTY tree-browser path. Indfetch add -i, users can no longer entersrc:orignore:values for archives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/add.py` around lines 87 - 108, The fallback currently yields an empty LsFunction (_empty) when subproject.as_vcs() is None, which causes the TTY tree-browser path to run and prevents _ask_src()/_ask_ignore() from falling back to text prompts; change the else branch to yield None (remove or stop using _empty) so callers detect "no VCS browser" and _ask_src()/_ask_ignore() can present the text prompt path (affecting dfetch add -i).
♻️ Duplicate comments (2)
dfetch/log.py (1)
67-70:⚠️ Potential issue | 🟠 MajorRemove inline pylint suppression at Line 70.
# pylint: disable=protected-accessindfetch/log.pyviolates the repository lint policy fordfetch/**/*.py. Please remove the inline suppression and route this through a small helper (or local adapter) so lint passes without suppression.Proposed fix (no inline suppression)
+def _set_handler_log_render(handler: RichHandler, render: Any) -> None: + """Install a custom Rich log renderer on a handler.""" + setattr(handler, "_log_render", render) + + def configure_root_logger(console: Console | None = None) -> None: @@ if os.getenv("ASCIINEMA_REC"): no_expand = _make_non_expanding_log_render( show_time=False, show_level=False, show_path=False ) - handler._log_render = no_expand # pylint: disable=protected-access + _set_handler_log_render(handler, no_expand)As per coding guidelines: "Do not use inline lint suppressions (# noqa, # type: ignore, # pylint: disable, # pyright: ignore) without fixing the root cause. Only module-level tool headers in test files (lines 1-5) are permitted."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/log.py` around lines 67 - 70, Remove the inline pylint suppression and instead encapsulate the protected attribute assignment in a small helper so linting is satisfied; create a helper function (e.g., set_handler_log_render(handler, render) or a local adapter) in dfetch/log.py that sets handler._log_render = render (or uses setattr) and call that helper where you currently call _make_non_expanding_log_render, replacing the direct protected-access line (handler._log_render = no_expand) with a call to the new helper; keep the rest of the logic unchanged and do not add any inline pylint disables.dfetch/commands/add.py (1)
275-291:⚠️ Potential issue | 🟠 MajorReuse
_resolve_entry_version()from the interactive--versionpath too.This helper fixes archive
--versionhandling only for_non_interactive_entry()._interactive_flow()still resolves overrides inline, sodfetch add -i <archive> --version foofalls back toVersion(branch="foo")instead of preserving the archive revision.Suggested follow-up in `_interactive_flow()`
if overrides.version is not None: - choices: list[Version] = [ - *[ - Version(branch=b) - for b in prioritise_default(branches, ctx.default_branch) - ], - *[Version(tag=t) for t in sort_tags_newest_first(tags)], - ] - version = _resolve_raw_version(overrides.version, choices) or Version( - branch=ctx.default_branch - ) + version = _resolve_entry_version(ctx, overrides.version) else: version = _ask_version(ctx.default_branch, branches, tags)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/add.py` around lines 275 - 291, Interactive flow currently resolves version overrides inline and doesn't use _resolve_entry_version, causing archive --version values to be treated as branches; update _interactive_flow to call _resolve_entry_version(ctx, raw_version) wherever it currently constructs Version(branch=raw_version) or resolves raw_version manually (compare to _non_interactive_entry usage) so archives preserve revision identifiers; ensure you pass the same _AddContext (ctx) and replace inline Version(...) creation with the returned Version object from _resolve_entry_version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/project/svnsubproject.py`:
- Around line 143-152: The _copy_license_files method only exports the first
detected file (license_files[0]) so additional files like NOTICE or COPYING are
skipped; update _copy_license_files to iterate over all entries returned by
SvnFetcher._license_files(root_branch_path) and call SvnRepo.export for each
file (use the same root_branch_path, revision and computed dest), ensuring dest
is computed once from local_path and used for every export.
- Around line 132-157: Add Google-style docstrings to the three private helpers
so pydocstyle is satisfied: for _apply_file_pattern(local_path, file_pattern,
source) add a short summary stating it removes files that don't match
file_pattern and warns if the directory is empty, then document Args
(local_path, file_pattern, source), Returns (None) and any side effects (deletes
files, logs warning); for _copy_license_files(local_path, branch_path, revision)
add a summary that it finds license files in the remote branch and exports the
first match into dest, then document Args (local_path, branch_path, revision),
Returns (None) and side effects (calls SvnRepo.export); for
_remove_ignored_files(local_path, ignore) add a summary that it removes matched
files/dirs except recognized license files, document Args (local_path, ignore),
Returns (None) and side effects (calls safe_rm), and include any raised
exceptions if applicable.
- Around line 69-79: In latest_available_version, don't assume any bare revision
pin is valid—validate the revision format (must be a numeric SVN revision)
before returning a Version; if wanted.revision is not an integer (e.g. contains
letters like "abc") return None instead of Version(...). Update the branch that
checks wanted.revision and not wanted.branch and not wanted.tag to
parse/regex-validate wanted.revision and only call
Version(revision=wanted.revision, branch=self.get_default_branch()) when
validation succeeds so callers won't treat invalid pins as available (fetch()
later rejects invalid revisions).
---
Outside diff comments:
In `@dfetch/commands/add.py`:
- Around line 87-108: The fallback currently yields an empty LsFunction (_empty)
when subproject.as_vcs() is None, which causes the TTY tree-browser path to run
and prevents _ask_src()/_ask_ignore() from falling back to text prompts; change
the else branch to yield None (remove or stop using _empty) so callers detect
"no VCS browser" and _ask_src()/_ask_ignore() can present the text prompt path
(affecting dfetch add -i).
---
Duplicate comments:
In `@dfetch/commands/add.py`:
- Around line 275-291: Interactive flow currently resolves version overrides
inline and doesn't use _resolve_entry_version, causing archive --version values
to be treated as branches; update _interactive_flow to call
_resolve_entry_version(ctx, raw_version) wherever it currently constructs
Version(branch=raw_version) or resolves raw_version manually (compare to
_non_interactive_entry usage) so archives preserve revision identifiers; ensure
you pass the same _AddContext (ctx) and replace inline Version(...) creation
with the returned Version object from _resolve_entry_version.
In `@dfetch/log.py`:
- Around line 67-70: Remove the inline pylint suppression and instead
encapsulate the protected attribute assignment in a small helper so linting is
satisfied; create a helper function (e.g., set_handler_log_render(handler,
render) or a local adapter) in dfetch/log.py that sets handler._log_render =
render (or uses setattr) and call that helper where you currently call
_make_non_expanding_log_render, replacing the direct protected-access line
(handler._log_render = no_expand) with a call to the new helper; keep the rest
of the logic unchanged and do not add any inline pylint disables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45636da9-cf90-4abf-af75-cec9161d7648
📒 Files selected for processing (4)
dfetch/commands/add.pydfetch/log.pydfetch/project/svnsubproject.pytests/test_subproject.py
Comment on lines
+69
to
+79
| def latest_available_version(self, wanted: Version) -> Version | None: | ||
| """Return the latest version matching *wanted*, or None if unavailable. | ||
|
|
||
| @staticmethod | ||
| def list_tool_info() -> None: | ||
| """Print out version information.""" | ||
| try: | ||
| tool, version = get_svn_version() | ||
| SubProject._log_tool(tool, version) | ||
| except RuntimeError as exc: | ||
| logger.debug( | ||
| f"Something went wrong trying to get the version of svn: {exc}" | ||
| ) | ||
| SubProject._log_tool("svn", "<not found in PATH>") | ||
| For revision-only pins (no branch, no tag) the pinned revision is | ||
| returned with the default branch so version comparison works correctly. | ||
| SVN revisions are globally ordered within a repository, so a bare | ||
| ``revision:`` in the manifest is always relative to trunk. | ||
| """ | ||
| if wanted.revision and not wanted.branch and not wanted.tag: | ||
| return Version(revision=wanted.revision, branch=self.get_default_branch()) | ||
| return super().latest_available_version(wanted) |
Contributor
There was a problem hiding this comment.
Validate bare SVN revisions before treating them as available.
Line 77 now returns a Version for any revision-only pin, including values like revision: abc that fetch() rejects later at Line 179. Invalid pins should return None here, otherwise callers can treat them as current until the later export step blows up.
Suggested fix
def latest_available_version(self, wanted: Version) -> Version | None:
"""Return the latest version matching *wanted*, or None if unavailable.
@@
SVN revisions are globally ordered within a repository, so a bare
``revision:`` in the manifest is always relative to trunk.
"""
if wanted.revision and not wanted.branch and not wanted.tag:
- return Version(revision=wanted.revision, branch=self.get_default_branch())
+ if not wanted.revision.isdigit():
+ return None
+ return Version(
+ revision=wanted.revision,
+ branch=self.get_default_branch(),
+ )
return super().latest_available_version(wanted)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/svnsubproject.py` around lines 69 - 79, In
latest_available_version, don't assume any bare revision pin is valid—validate
the revision format (must be a numeric SVN revision) before returning a Version;
if wanted.revision is not an integer (e.g. contains letters like "abc") return
None instead of Version(...). Update the branch that checks wanted.revision and
not wanted.branch and not wanted.tag to parse/regex-validate wanted.revision and
only call Version(revision=wanted.revision, branch=self.get_default_branch())
when validation succeeds so callers won't treat invalid pins as available
(fetch() later rejects invalid revisions).
Comment on lines
+132
to
+157
| def _apply_file_pattern( | ||
| self, local_path: str, file_pattern: str, source: str | ||
| ) -> None: | ||
| for file in find_non_matching_files(local_path, (file_pattern,)): | ||
| os.remove(file) | ||
| if not os.listdir(local_path): | ||
| logger.warning( | ||
| f"The 'src:' filter '{source}' didn't match any files" | ||
| f" from '{self._remote}'" | ||
| ) | ||
|
|
||
| complete_path = "/".join( | ||
| filter(None, [self.remote, branch_path.strip(), self.source]) | ||
| ).strip("/") | ||
| def _copy_license_files( | ||
| self, local_path: str, branch_path: str, revision: str | ||
| ) -> None: | ||
| root_branch_path = "/".join([self._remote, branch_path]).strip("/") | ||
| license_files = SvnFetcher._license_files(root_branch_path) | ||
| if license_files: | ||
| dest = ( | ||
| local_path if os.path.isdir(local_path) else os.path.dirname(local_path) | ||
| ) | ||
| SvnRepo.export(f"{root_branch_path}/{license_files[0]}", revision, dest) | ||
|
|
||
| # When exporting a file, the destination directory must already exist | ||
| pathlib.Path(os.path.dirname(self.local_path)).mkdir( | ||
| parents=True, exist_ok=True | ||
| ) | ||
| def _remove_ignored_files(self, local_path: str, ignore: Sequence[str]) -> None: | ||
| for file_or_dir in find_matching_files(local_path, ignore): | ||
| if not (file_or_dir.is_file() and is_license_file(file_or_dir.name)): | ||
| safe_rm(file_or_dir) |
Contributor
There was a problem hiding this comment.
Add docstrings to the new private helpers.
_apply_file_pattern(), _copy_license_files(), and _remove_ignored_files() are in dfetch/ but currently have no docstrings, so this refactor will keep pydocstyle complaining about the module.
As per coding guidelines, "Follow Google-style docstring conventions, enforced by pydocstyle on dfetch/ (but not on tests/)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/svnsubproject.py` around lines 132 - 157, Add Google-style
docstrings to the three private helpers so pydocstyle is satisfied: for
_apply_file_pattern(local_path, file_pattern, source) add a short summary
stating it removes files that don't match file_pattern and warns if the
directory is empty, then document Args (local_path, file_pattern, source),
Returns (None) and any side effects (deletes files, logs warning); for
_copy_license_files(local_path, branch_path, revision) add a summary that it
finds license files in the remote branch and exports the first match into dest,
then document Args (local_path, branch_path, revision), Returns (None) and side
effects (calls SvnRepo.export); for _remove_ignored_files(local_path, ignore)
add a summary that it removes matched files/dirs except recognized license
files, document Args (local_path, ignore), Returns (None) and side effects
(calls safe_rm), and include any raised exceptions if applicable.
Comment on lines
+143
to
+152
| def _copy_license_files( | ||
| self, local_path: str, branch_path: str, revision: str | ||
| ) -> None: | ||
| root_branch_path = "/".join([self._remote, branch_path]).strip("/") | ||
| license_files = SvnFetcher._license_files(root_branch_path) | ||
| if license_files: | ||
| dest = ( | ||
| local_path if os.path.isdir(local_path) else os.path.dirname(local_path) | ||
| ) | ||
| SvnRepo.export(f"{root_branch_path}/{license_files[0]}", revision, dest) |
Contributor
There was a problem hiding this comment.
Copy every detected license file, not just the first one.
Line 152 exports only license_files[0]. Repositories that ship LICENSE plus NOTICE/COPYING will lose the extra notice whenever src: filtering is used.
Suggested fix
if license_files:
dest = (
local_path if os.path.isdir(local_path) else os.path.dirname(local_path)
)
- SvnRepo.export(f"{root_branch_path}/{license_files[0]}", revision, dest)
+ for license_file in license_files:
+ SvnRepo.export(f"{root_branch_path}/{license_file}", revision, dest)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/project/svnsubproject.py` around lines 143 - 152, The
_copy_license_files method only exports the first detected file
(license_files[0]) so additional files like NOTICE or COPYING are skipped;
update _copy_license_files to iterate over all entries returned by
SvnFetcher._license_files(root_branch_path) and call SvnRepo.export for each
file (use the same root_branch_path, revision and computed dest), ensuring dest
is computed once from local_path and used for every export.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SubProject was an ABC mixing VCS-specific capabilities with domain
orchestration. Archive fetching has no concept of branches/tags/revisions,
so forcing it to implement VCS-shaped stubs was a design smell.
New domain model:
latest_available_version, list_tool_info)
browse_tree, patch_type) — git and svn only
eliminating duplication between GitFetcher and SvnFetcher
dispatch point via as_vcs() -> VcsFetcher | None
All cyclomatic complexity kept ≤ 6. Pylint 10.00/10. All 569 unit tests
pass. Feature tests pass for git; SVN failures are environmental (no svn).
https://claude.ai/code/session_01BMSF8XFAxV6hABQgL7RZ3z
Summary by CodeRabbit
Refactor
Tests
Chores